Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

improvement: Make including detail in completion label configurable #6986

Merged
merged 2 commits into from
Dec 6, 2024

Conversation

Sorixelle
Copy link
Contributor

@Sorixelle Sorixelle commented Nov 29, 2024

Some LSP clients (such as Emacs using Eglot and Corfu) use the label field to insert a completion if completionItem/resolve has not returned by the time the completion is selected. Including the contents of the detail property in the label causes it to be included in the file itself in these clients. To remedy this, we define a isDetailIncludedInLabel option under compilerOptions to make this behaviour toggleable. We also disable this behaviour by default in Emacs, since this is where the undesirable behaviour was located.

TODO:

  • Figure out why option is ignored in Scala 3.3.4 projects
  • Testing (unsure what tests should be added - please let me know where they should go)

Fixes #6849.

@Sorixelle
Copy link
Contributor Author

Sorixelle commented Nov 29, 2024

Seems like the changes to MetalsServerConfig.default don't get applied to Scala 3.3.4 projects. Tested with a Mill project using 3.3.4 and the option is not automatically applied - I need to configure my editor to set it explicitly during the initialize command. Bad test case on my end, it seems like the option is not honored at all in 3.3.4. Unsure why this is the case - any pointers?

EDIT: Some investigation leads me to believe that Metals is falling back to the Scala presentation compiler instead of mtags. Unsure if there's anything that can be done about that until mtags is updated for 3.3.4.

@tgodzik
Copy link
Contributor

tgodzik commented Nov 29, 2024

EDIT: Some investigation leads me to believe that Metals is falling back to the Scala presentation compiler instead of mtags. Unsure if there's anything that can be done about that until mtags is updated for 3.3.4.

From 3.3.4 onwards we use mtags published by the compiler itself to avoid having to publish it to every version of the compiler.

@tgodzik
Copy link
Contributor

tgodzik commented Nov 29, 2024

Ach, and we usually just port them ourselves, so no need to worry about it

Copy link
Contributor

@tgodzik tgodzik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good aside from the MiMa failure 🎉

@Sorixelle Sorixelle force-pushed the label-detail-config branch 2 times, most recently from 80387b5 to fad1b04 Compare November 29, 2024 18:06
@Sorixelle Sorixelle marked this pull request as ready for review November 29, 2024 18:06
@Sorixelle
Copy link
Contributor Author

Addressed the default - are we sure this change doesn't require any tests? I'd add them, I'm just not sure where they belong.

@tgodzik
Copy link
Contributor

tgodzik commented Dec 1, 2024

Addressed the default - are we sure this change doesn't require any tests? I'd add them, I'm just not sure where they belong.

You should be able to add the tests to the cross module, you would need to change the config like in https://github.com/scalameta/metals/blob/main/tests/cross/src/test/scala/tests/pc/CompletionSnippetNegSuite.scala#L10

Some LSP clients (such as Emacs using Eglot and Corfu) use the label field to insert a completion if
completionItem/resolve has not returned by the time the completion is selected. Including the contents of the detail
property in the label causes it to be included in the file itself in these clients. To remedy this, we define a
isDetailIncludedInLabel option under compilerOptions to make this behaviour toggleable. We also disable this behaviour
by default in Emacs, since this is where the undesirable behaviour was located.

Fixes scalameta#6849.
@Sorixelle Sorixelle force-pushed the label-detail-config branch from 041ab36 to f0ccc7b Compare December 2, 2024 20:42
@Sorixelle
Copy link
Contributor Author

That should do for test cases - let me know if there's anything else you think I should check.

@tgodzik
Copy link
Contributor

tgodzik commented Dec 5, 2024

That should do for test cases - let me know if there's anything else you think I should check.

I added one test, fixed the added one on Scala 3 and removed the detail also from scope completions. Thanks for working on this!

Copy link
Contributor

@tgodzik tgodzik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

I will merge tomorrow once it passes (unless I broke something)

@tgodzik tgodzik merged commit b5e5e84 into scalameta:main Dec 6, 2024
22 checks passed
@Sorixelle Sorixelle deleted the label-detail-config branch December 6, 2024 12:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Details is duplicated into label in completions, causing odd behaviour in some editor setups
2 participants